chore(kernel): bump KERNEL_REV for getTypeInfo parity - #512
Merged
Conversation
Signed-off-by: Cathleen Yan <cathleen.yan@databricks.com>
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 High
Needs a fix before merge: the loader was regenerated to require kernel binding 0.2.1, but package.json optionalDependencies (and package-lock.json) still pin the native packages to 0.2.0. This means the advertised getTypeInfo() parity fix won't actually be installed, and enabling NAPI_RS_ENFORCE_VERSION_CHECK will throw a version-mismatch error. The CHANGELOG and KERNEL_REV changes themselves look correct.
Other findings
- 🟠 High — The regenerated loader now enforces
0.2.1for every native binding (e.g.if (bindingPackageVersion !== '0.2.1' && process.env.NAPI_RS_ENFORCE_VERSION_CHECK ...)), butoptionalDependencieshere still pin all@databricks/databricks-sql-kernel-*packages to0.2.0.package-lock.jsonlikewise still resolves them to0.2.0.
Two consequences:
- The parity fix does not ship. The
getTypeInfo()fix documented in this PR lives in the0.2.1native package, but a freshnpm installwill still fetch and load0.2.0— so the behavior this PR advertises won't actually be present for consumers. - Hard failure when the version check is enabled. With
NAPI_RS_ENFORCE_VERSION_CHECKset to a non-0value, the loader will throwNative binding package version mismatch, expected 0.2.1 but got 0.2.0because the installed optional dep is0.2.0.
The optionalDependencies pins (all 8 entries) and the corresponding package-lock.json entries should be bumped to 0.2.1 to match the loader and KERNEL_REV.
Signed-off-by: Cathleen Yan <cathleen.yan@databricks.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
KERNEL_REVto the merged databricks-sql-kernel#291 commit so source-built native bindings include thegetTypeInfo()parity fix0.2.0, the latest currently published native package versionRelease note
This pin makes the fix available to source builds. Customer-facing npm installs still resolve the
0.2.0platform packages, which do not contain Kernel #291. Once a native package containing that change is published, a follow-up must update all eight optional dependencies,package-lock.json, and the generated loader together.Validation
version()returns0.2.1npm run buildnpm test -- tests/unit/kernel/execution.test.ts(68 passing)list_type_info